-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
trace2: remove inaccurate warning and add UI helper exit calls #1114
trace2: remove inaccurate warning and add UI helper exit calls #1114
Conversation
Remove warning about being unable to set up tracing from Trace2 TryParseSettings method. This method should just check to see whether TRACE2 is enabled - if it is not, it does not need to warn the user, as we only collect traces from those who actively choose to opt in.
c625f9e
to
b7b24a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions on thread naming.
@@ -11,6 +12,9 @@ public static class Program | |||
{ | |||
public static void Main(string[] args) | |||
{ | |||
// Name the main thread of execution for TRACE2 tracing | |||
Thread.CurrentThread.Name ??= "main"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if .NET Framework support the syntax sugar for ??=
.
Also, does this mean if there's already a name we don't set it to main
? Why not just always set it to main
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just used the suggestion from this documentation, but maybe that's being overly safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that for helper UI apps we reserve the entry/primary thread for the UI dispatcher.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is important that the main thread (the one sending the "version", "start" and "exit" events be called "main" so that I can special case other items that are associated with it.
are UI helper apps spawned as child processes from the "main" thread? that model works for me. Git's hook processes work that way. Really, all of Git creates children from the main thread -- threads are only really used for parallel algorithms IIRC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct! Thanks for confirming. We will stick with this name, but I will remove the null check per @mjcheetham's comment.
This PR is just adding thread names to the existing version, start, and exit events, right? If we assume that version, start, and exit will only be called on the main thread, you My concern is that later we need to think about C# thread usage -- lambdas and asyncs Then again, maybe we won't need to instrument GCM to that level, so it might not |
Yes.
Yes.
The problem is we make heavy use of We don't actually spin up any 'real' threads explicitly(*), that I can remember off the top of my head. We only spin up child processes, which we're already handling. (*) Caveat this with the fact for the UI helpers we really do explicitly spin up a new thread to run the 'main' application, and reserve the actual entry thread to run the UI dispatcher/message pump. |
Based on what Jeff said above, I think we should probably call the UI Helper threads |
GCM's UI helpers are connected to the TRACE2 system via the Start() call in ApplicationBase.cs. Since this call records Version and Start events, ensure a corresponding Exit event is called before the helper process completes.
b7b24a7
to
6b2459c
Compare
@mjcheetham and I spoke offline and determined that the addition of thread names is going to require a bit more design than originally anticipated. It also may need to wait for the in-process UI helper work to land. I went ahead and removed that commit in light of this. |
Let's not worry about the async pool threads right now. They only become significant if there are region-enter and -leave events being generated in the async proc / lamba body. If we're only interested in logging child process events, they can all just belong to "main". and if the actual entry thread is just running the msg pump and you create a real thread to run the app, just let that new thread be "main" (assuming it will be the one doing all of the work and sending those 3 events). |
@mjcheetham and I discussed this yesterday and landed on not renaming this thread (naming it "main" could be misleading since it's not actually the main thread). I thought about it and, as a compromise for now, I'm going to just hardcode the |
The TRACE2 convention for thread naming is for each child process's main thread of execution to be named "main." In GCM, however, we encountered an issue with our UI Helper exes - their main threads of execution are named AppMain. To temporarily work around this issue, we default the thread name for all TRACE2 events to "main" (rather than changing GCM's process names). This will do for our current events, which are all called from the main thread of execution. However, it is important to note that future events (i.e. regions) will require deeper thought around the GCM/TRACE2 process model, as they will be called on threads from .NET's ThreadPool rather than the main thread of execution.
Remove warning about being unable to set up tracing from Trace2 TryParseSettings method. This method should just check to see whether TRACE2 is enabled - if it is not, it does not need to warn the user, as we only collect traces from those who actively choose to opt in.
Additionally, GCM's UI helpers are connected to the TRACE2 system via the Start() call in
ApplicationBase.cs
. Since this call recordsVersion
andStart
events, ensure a correspondingExit
event is called before the helper process completes.